Summary

The @volatile var + reset-under-lock + local-capture-at-await-site pattern landed in 4101068 closes the PR's targeted failure mode but leaves two unaddressed TOCTOU windows on the same latch lifecycle. Both windows are reachable via realistic host-app + SyncJobService interleavings, and both re-introduce the contract violations this PR was specifically designed to fix.

(1) Completer-side wrong-latch race

notifyInitComplete() (OneSignalImp.kt:483-485) is:

private fun notifyInitComplete() {
    suspendCompletion.complete(Unit)
}

It reads the @Volatile field at .complete() time. The await sites local-capture under initLock, but the completer side does not — so it can complete a different generation of the latch than the one it was supposed to release. The non-atomic pair initState = FAILED + notifyInitComplete() appears at three sites:

• L358-361 (user-locked branch)
• L378-381 (missing appId branch)
• L402-403 (catch block)

All three live outside any synchronized block. isSDKAccessible() returns false for FAILED, so a concurrent caller landing in the synchronized block at L678-693 (or L305-315) is allowed to transition state and reset the latch.

Step-by-step proof

Initial state: initState = NOT_STARTED, suspendCompletion = D0.

1. T_A calls initWithContextSuspend(ctx, "appId"). Synchronized block flips initState = IN_PROGRESS, allocates suspendCompletion = D1. Releases lock. Begins internalInit.
2. T_C (re-entrant suspend caller, e.g. SyncJobService.onStartJob → initWithContext(this) → initWithContextSuspend(ctx, null)) enters the synchronized block at L678. Observes isSDKAccessible() == true (state is IN_PROGRESS), captures completionToAwait = suspendCompletion = D1. Releases lock. Suspends on D1.await() at L703.
3. T_A: internalInit body throws (e.g. inside bootstrapServices iterating registered IBootstrapService implementations). Catch block executes at L398-404. Reaches L402: initState = InitState.FAILED. ⏸️ (thread preemption, GC pause, etc.)
4. T_B (separate thread, e.g. host-app retry OneSignal.initWithContext(ctx, "appId")) enters the synchronized block at L305 (sync path) or L678 (suspend path). Observes initState == FAILED, !isSDKAccessible(). Flips initState = IN_PROGRESS, allocates suspendCompletion = D2. Releases lock. Spawns its own internalInit.
5. T_A: resumes at L403 → notifyInitComplete() → reads @Volatile suspendCompletion = D2 → D2.complete(Unit).

Outcomes

• D1 is never completed. T_C hangs on D1.await() forever — the exact SyncJobService budget-slot leak this PR was meant to prevent.
• D2 is completed prematurely. Any waiter that captures D2 after T_B's reset wakes immediately. In initWithContextSuspend it returns initState == SUCCESS against transient IN_PROGRESS → false, silently dropping JobService work. In waitUntilInitInternal the L548 FAILED check fails, the function falls through past the misleading "// initState is guaranteed to be SUCCESS here" comment, and the caller proceeds to getter() against a service whose retry-init bootstrap has not run.

The SUCCESS path at L389-390 is safe because state == SUCCESS keeps isSDKAccessible() == true, so no concurrent caller can reset the latch.

(2) Awaiter-side stale-state race

The symmetric defect is on the awaiter side. After completionToAwait!!.await() returns, both call sites re-read live volatile initState outside the lock:

• waitUntilInitInternal: L536 (await) → L548 (if (initState == InitState.FAILED))
• initWithContextSuspend: L703 (await) → L704 (return@withContext initState == InitState.SUCCESS)

The gap between latch completion and continuation resumption depends on coroutine dispatcher scheduling and can be substantial. A concurrent retry-after-FAILED during that gap flips state from FAILED to IN_PROGRESS, defeating the post-await check. This race exists even when notifyInitComplete() correctly completes the right latch — i.e. it is independent of the bug above.

For initWithContextSuspend, returning false on transient IN_PROGRESS is a silent JobService work drop. For waitUntilInitInternal, falling through past the FAILED check on transient IN_PROGRESS is the SessionService-NPE class — the caller invokes getter() against services whose retry init has not bootstrapped (configModel.appId not set, userSwitcher.initUser not called, startupService.scheduleStart not run).

Why the PR's regression tests don't cover this

• initWithContextSuspend resets latch on retry-after-FAILED serializes: firstInit shouldBe false runs to completion before the second init starts.
• initWithContextSuspend reaches terminal state when internalInit throws is single-threaded.
• The original in-flight init waits for completion test does not exercise FAILED retry at all.

Neither test launches a thread between initState = FAILED and notifyInitComplete(), and none launch a re-entrant caller between await() returning and the post-await state read.

Suggested fix

A single change addresses both races:

1. Change the latch payload to encode the terminal state: CompletableDeferred<InitState>.
2. At the start of internalInit, capture val myCompletion = suspendCompletion (under or just after the lock that reset it).
3. All terminal-state paths call myCompletion.complete(InitState.FAILED) or myCompletion.complete(InitState.SUCCESS) on the captured local — never on the volatile field.
4. Awaiter sites use the awaited result (val terminalState = completionToAwait!!.await()) instead of re-reading volatile initState.

This pattern eliminates both windows by making the terminal state of each generation observable as the latch payload, decoupling it from the live volatile field that concurrent retries mutate.

Alternatively, holding initLock across initState = FAILED and notifyInitComplete() closes window (1) but does not close window (2); window (2) requires either the result-in-payload pattern or a loop that re-captures and re-awaits while observed state is IN_PROGRESS.

What the bug is

The PR added a try/catch around internalInit's body (with a comment explicitly naming the hang failure mode it prevents) and a fresh-latch reset under synchronized(initLock) at the start of every init attempt. But on the sync initWithContext(context, appId) entry path, there is unprotected code between the latch reset and the internalInit call. That gap re-introduces the very hang the catch block was designed to prevent.

Code path

OneSignalImp.kt lines 297-333:

override fun initWithContext(context: Context, appId: String): Boolean {
    synchronized(initLock) {
        if (initState.isSDKAccessible()) { return true }
        initState = InitState.IN_PROGRESS
        suspendCompletion = CompletableDeferred()  // L1: fresh latch
    }
    ensureApplicationServiceStarted(context)       // <-- line 319, OUTSIDE try/catch
    if (isBackgroundThreadingEnabled) {
        suspendifyOnIO { internalInit(context, appId) }  // catch lives inside internalInit
        return true
    }
    return runBlocking(runtimeIoDispatcher) { internalInit(context, appId) }
}

ensureApplicationServiceStarted reaches ApplicationService.start(context) (ApplicationService.kt line 78), which does:

val application = context.applicationContext as Application  // line 81

That cast throws ClassCastException for any Context whose applicationContext is not an Application instance — uncommon in production but realistic for hosts initializing from ContentProvider contexts (some apps do this for pre-onCreate init), test/instrumentation harnesses that wrap or mock Context, or restricted/launcher contexts that return a ContextWrapper.

Why existing code does not prevent it

The PR's new try { ... } catch (t: Throwable) { ... initState = FAILED; notifyInitComplete() ... } is scoped to internalInit's body (OneSignalImp.kt lines ~360-405). It does not wrap the lines between the synchronized block and the internalInit invocation in the sync entry path. The PR comment on that catch even enumerates the exact symptom — "would otherwise leave initState at IN_PROGRESS forever and suspendCompletion uncompleted -- causing re-entrant suspend callers (e.g. SyncJobService) to hang indefinitely on await()" — but the protection is one stack frame too narrow.

isBackgroundThreadingEnabled itself is already defensively wrapped (catches Throwable from the feature-manager lookup), so the only realistic throw point in the unprotected window is ensureApplicationServiceStarted.

Step-by-step proof

1. Host MainApplication.onCreate() calls OneSignal.initWithContext(weirdContext, "appId") where weirdContext.applicationContext is a ContextWrapper (not an Application).
2. Synchronized block (lines 305-315): initState = IN_PROGRESS, suspendCompletion = L1 (fresh).
3. Line 319: ensureApplicationServiceStarted(weirdContext) → (applicationService as ApplicationService).start(weirdContext) → val application = weirdContext.applicationContext as Application throws ClassCastException.
4. Throw escapes initWithContext. SDK state: initState=IN_PROGRESS, suspendCompletion=L1 uncompleted, applicationServiceStarted=false. Host catches the throw (or the process keeps running because Application onCreate exceptions are sometimes survivable, or a persisted JobService entry outlives the throwing process invocation).
5. Later, SyncJobService.onStartJob fires and calls OneSignal.initWithContext(this) (suspend, no-appId) → initWithContextSuspend(ctx, null).
6. Synchronized block at lines 678-693: isSDKAccessible() is true (IN_PROGRESS counts). shouldRunInit=false, captures completionToAwait = suspendCompletion = L1.
7. Line 703: completionToAwait!!.await() — blocks forever on L1, which no code path will ever complete because no internalInit was invoked for this generation.
8. SyncJobService coroutine hangs until the OS kills the worker, holding its JobService budget slot — the same hang failure mode the PR's catch block was supposed to prevent.

The same hang affects waitUntilInitInternal (line 533): any subsequent accessor call (os.user, os.notifications, etc.) under SDK_BACKGROUND_THREADING blocks forever in runBlocking { waitUntilInitInternal() } -> await() on L1.

Why this is a regression introduced by this PR

• Pre-PR: suspendCompletion was a single-shot val; initWithContextSuspend did not await() on it. A re-entrant suspend caller observing isSDKAccessible() == true early-returned true without awaiting (the SDK-4475 NPE this PR fixes). A leaked latch was harmless because nothing awaited.
• Post-PR: re-entrant suspend callers now await() on the freshly-reset latch (line 703), and waitUntilInitInternal does the same (line 533). A leaked latch from a pre-internalInit throw turns the prior NPE-on-stale-state into an indefinite hang. The bug class shifts from "returns too early" to "never returns" — exactly the inversion the PR's internalInit catch was designed to prevent.

Impact

Trigger probability is narrow (most hosts pass an Activity / Application / Service context whose applicationContext is the Application), but the consequence is severe and matches the precise hang the PR is attempting to close. SyncJobService callers hold JobService budget slots until the OS terminates the worker; accessor calls under SDK_BACKGROUND_THREADING will block forever. Importantly, the PR's defensive intent is explicit in the new catch comment, so this is a literal gap in the same defensive pattern rather than a hypothetical future hardening.

Suggested fix

Mirror the internalInit catch around the sync-entry body. Two equivalent options:

Option A — wrap the post-lock body:

synchronized(initLock) { ... }
try {
    ensureApplicationServiceStarted(context)
    if (isBackgroundThreadingEnabled) {
        suspendifyOnIO { internalInit(context, appId) }
        return true
    }
    return runBlocking(runtimeIoDispatcher) { internalInit(context, appId) }
} catch (t: Throwable) {
    initFailureException = (initFailureException ?: IllegalStateException(...)).apply { addSuppressed(t) }
    initState = InitState.FAILED
    notifyInitComplete()
    throw t
}

Option B — move ensureApplicationServiceStarted into internalInit so it falls under the existing try/catch. Would need to preserve the order with the isBackgroundThreadingEnabled evaluation (which currently relies on applicationServiceStarted being true to avoid returning false prematurely).

What changed

Pre-PR (per the diff context), initWithContextSuspend constructed and assigned initFailureException immediately on entry, before withContext(runtimeIoDispatcher):

// pre-PR
initFailureException = IllegalStateException("OneSignal initWithContext failed.")
// This ensures the stack trace points to the caller that triggered init, not the async worker thread.
return withContext(runtimeIoDispatcher) { ... }

Post-PR (OneSignalImp.kt:691), the assignment lives inside the shouldRunInit branch of the synchronized(initLock) block — which is itself inside withContext(runtimeIoDispatcher). The constructor therefore now runs on an IO worker continuation, and the captured stack trace contains IO dispatcher frames rather than the caller's coroutine continuation frames. The pre-PR comment that documented the invariant at the assignment site was deleted, but the field-level comment at OneSignalImp.kt:62 still asserts it: Save the exception pointing to the caller that triggered init, not the async worker thread.

Why the change happened

This was a deliberate response to Copilot inline-comment 3196748195: initFailureException is overwritten at the start of every initWithContextSuspend call, even when this invocation does not actually start init. The fix correctly gates the assignment so re-entrant suspend callers (notably the SyncJobService entry point) no longer overwrite the original initiator's failure-attribution exception. That part is sound. The accidental side effect is that hoisting the assignment under the lock also relocated the constructor into the IO continuation.

Practical impact (why this is a nit, not a blocker)

1. The substantive diagnostic info is preserved. The exception's message (OneSignal initWithContext failed.) is unchanged, and the suppressed cause attached in internalInit (the addSuppressed paths at lines ~691 and ~702) carries the actual failure-site stack into the bootstrap path. Crash reporters and debuggers surfacing this exception still get the meaningful information.
2. Pre-PR, the "caller's stack" guarantee was already weak on the suspend path. initWithContextSuspend is a suspend function — its callers are already in a coroutine, so the JVM stack at the constructor call site (whether before or after withContext) is dominated by continuation/dispatcher machinery; the user's actual code site (e.g. launch { os.initWithContextSuspend(...) }) is not present at any point. The deleted comment overstated what was actually preserved.
3. The typical sync initWithContext(ctx, appId) path is unaffected — that path doesn't touch initFailureException at all (only the catch block in internalInit does, on whichever runner thread).
4. The existing test accessor instances after initWithContext without appID shows the failure reason (SDKInitTests.kt:75-83) only asserts on .message and suppressed[0].message, not on the stack trace, so the regression is not test-detected.

Step-by-step proof of the regression

1. Host calls os.initWithContextSuspend(context) from inside a coroutine on the Main dispatcher (or from runBlocking on a non-coroutine thread).
2. Execution enters initWithContextSuspend body. Pre-PR, IllegalStateException(...) is constructed here — its captured stack contains the coroutine's continuation frames on the Main dispatcher (or the runBlocking thread). Post-PR, no construction happens yet.
3. withContext(runtimeIoDispatcher) switches to an IO dispatcher worker. Post-PR, the synchronized block runs here, and IllegalStateException(...) is constructed now — its captured stack contains IO worker continuation frames.
4. Init eventually fails. Some later waitUntilInitInternal caller throws initFailureException. Pre-PR: stack trace shows Main dispatcher / runBlocking-thread continuation. Post-PR: stack trace shows IO worker continuation.
5. Both stacks are coroutine machinery; neither cleanly points at the host app's call site. But pre-PR was at least consistent with the documented invariant of "caller's dispatcher," whereas post-PR is unambiguously "IO worker."

Suggested fix (if you want to honor the documented invariant)

Hoist the constructor above withContext and only the assignment under the lock. The two concerns (no-overwrite and caller-dispatcher capture) are orthogonal:

override suspend fun initWithContextSuspend(...): Boolean {
    Logging.log(...)
    // Capture stack on caller's dispatcher.
    val newFailureEx = IllegalStateException("OneSignal initWithContext failed.")
    return withContext(runtimeIoDispatcher) {
        synchronized(initLock) {
            if (initState.isSDKAccessible()) { ... }
            else {
                initState = InitState.IN_PROGRESS
                suspendCompletion = CompletableDeferred()
                initFailureException = newFailureEx  // assignment under lock; stack captured outside withContext
            }
        }
        ...
    }
}

Or alternately: drop the field-level comment to match what's actually preserved (message + suppressed cause), since the "caller frames" claim was already largely aspirational on a suspend path. Either fix is fine; the current code is internally inconsistent.